-
Notifications
You must be signed in to change notification settings - Fork 39
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(session replay): delay compresion #885
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really cool. I had no idea about requestIdleCallback
before the PR, it seems like a good use case for this. I'm wondering though if we want to change this a little bit. The things that come to mind for me:
- The hard coded timeout seems like it'll fire a lot of events just slightly delayed. Therefore not immediately impacting performance, but just having it delayed. I think it being hard coded is to going to be slightly problematic since that number will vary from app to app.
- It seems like we're creating a new callback for each event. I'm not entirely certain it's a problem, but for a high number of events this could be a lot of callbacks.
I'm thinking to help mitigate some of these things maybe we could instead of a queue that we work through during idle periods and get through as many as possible. We could probably use the IdleDeadline object to know when to quit. The idea being that we register a single callback that processes through the queue as much as possible during idle periods. Getting this exactly right and not going over time may be tough, but I think this would be more performant.
I think in either case before we merge this we should add an option so that we can test this ourselves for now and make sure we add some performance charts with a sample app or preferably our site so we're confident in the change.
I'm definitely open to feedback/other thoughts on this!
Thanks for the feedback!
Agreed on adding an option so we can have control over testing this, and we can let customers know this is something they could turn on. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I think there are minor changes we can make, but with the config in place and the current approach I don't see anything majorly blocking here.
Summary
Delays the compression of events emitted by rrrweb record using requestIdleCallback.
Checklist